Conversation
vharseko
added
bug
concurrency
maximthomas
left a comment
maximthomas
left a comment
There was a problem hiding this comment.
praise: The change puts the decision and the report outside the operation the storage may replay, which is the shape #991 needs.
planIndexUpdates(AttributeIndex.java:1038) decides from the pre-change state, so a replayed untrust write reaches the same answer on every attempt.- The inner catch is gone from
VLVIndex.java:281-293: a conflict inside the untrust operation now reaches the storage's retry loop — on PDB and on JDBC — instead of committing an attempt which did nothing. conflictAtCommitOnWrite(ReplayedConfigChangeTest.java:924) makes a change with several writes testable, and the three new cases are red at the base for the right assertion each.
issue (blocking): The rebuild instruction is decided before the write and delivered only if the write commits.
opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/AttributeIndex.java:1006-1016, :942-953
opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/VLVIndex.java:290-292, :312-316
When the storage gives the write up (PDB after its retry budget, #937; JDBC after MAX_RETRIES), catch (Exception) at :1012 adds the server error and a stack trace; rebuildMessages + setAdminActionRequired(true) (:1006-1010) and the limit loop (:1002-1005) are skipped. What that road leaves: the index untrusted in memory (DefaultIndex.setTrusted :324 assigns before the tree write), TRUSTED intact on disk, and config.ldif already holding the raised limit — ConfigurationHandler.replaceEntry writes it at :642, before the listener loop, and nothing rolls it back. The log line says adminActionRequired=false, the client gets ERR_CONFIG_FILE_MODIFY_APPLY_FAILED with a stack trace naming no index, and the next restart trusts the index under the raised limit: the #991 end state, on a road where the base delivered NOTE_CONFIG_INDEX_ENTRY_LIMIT_REQUIRES_REBUILD and adminActionRequired=true. Write 1 has the same shape for the added indexes. In VLVIndex the :290 catch rethrows, so the result carrying adminActionRequired=true (:248/:253/:263) is discarded whole.
The instruction is right on both roads, so say it before the write; only the limit belongs after the commit:
planIndexUpdates(updatedIndexes.values(), newConfiguration, indexesToUntrust, rebuildMessages);
for (LocalizableMessage rebuildMessage : rebuildMessages)
{
ccr.setAdminActionRequired(true);
ccr.addMessage(rebuildMessage);
}
entryContainer.getRootContainer().getStorage().write(/* untrust only, as now */);
for (final Index updatedIndex : updatedIndexes.values())
{
updatedIndex.setIndexEntryLimit(newConfiguration.getIndexEntryLimit());
}For VLVIndex, the :290 catch can report the way AttributeIndex :1012 does instead of throwing:
catch (final Exception e)
{
ccr.addMessage(NOTE_INDEX_ADD_REQUIRES_REBUILD.get(getName()));
ccr.addMessage(LocalizableMessage.raw(StaticUtils.stackTraceToSingleLineString(e)));
ccr.setResultCode(DirectoryServer.getCoreConfigManager().getServerErrorResultCode());
return ccr;
}Pin: the NO_REPLAY twin of conflictAtCommitOnWrite in the harness, and one case per class on the failed road — red at the head (adminActionRequired false, no ordinal), green with the fix:
void failWithoutReplayOnWrite(int nth)
{
arm(ConflictPoint.NO_REPLAY, 1);
armedWrite = writes + nth;
}
backend.storage.failWithoutReplayOnWrite(3);
final ConfigChangeResult ccr = index.applyConfigurationChange(indexCfg(newTreeSet(IndexType.EQUALITY), 8000));
assertThat(ccr.getResultCode()).isNotEqualTo(ResultCode.SUCCESS);
assertThat(ccr.adminActionRequired()).as("the rebuild the raised limit needs, on the road which failed").isTrue();
assertThat(ordinalsOf(ccr)).contains(NOTE_CONFIG_INDEX_ENTRY_LIMIT_REQUIRES_REBUILD.ordinal());
assertThat(persistedFlags(rootContainer, ec, cnIndex.getName())).as("what the restart will read").contains(TRUSTED);suggestion (non-blocking): The untrust write in AttributeIndex opens a transaction with nothing to do.
opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/AttributeIndex.java:991
storage.write runs with an empty indexesToUntrust — a lowered limit, a confidentiality change which needs nothing — where VLVIndex.java:271 guards it and says why: a bounded storage can give that write up with nothing to give up, which puts the blocking issue's road in front of a change that asks for nothing. Pre-existing, but the PR aligns the two classes.
if (!indexesToUntrust.isEmpty())
{
entryContainer.getRootContainer().getStorage().write(...);
}suggestion (non-blocking): No case lowers or keeps the entry limit, so < → != at the comparison survives every case.
opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/AttributeIndex.java:1044, ReplayedConfigChangeTest.java:616-650
Every AttributeIndex case raises the limit (4000 → 8000). The mutant untrusts and reports on a lowered limit and nobody is red. A pre-existing gap, re-homed by the PR with the new cases around it.
final ConfigChangeResult ccr = index.applyConfigurationChange(indexCfg(newTreeSet(IndexType.EQUALITY), 2000));
assertThat(ccr.adminActionRequired()).as("a lowered limit needs no rebuild").isFalse();
assertThat(ccr.getMessages()).isEmpty();
assertThat(cnIndex.isTrusted()).isTrue();
assertThat(cnIndex.getIndexEntryLimit()).isEqualTo(2000);suggestion (non-blocking): "A change asking for nothing opens no transaction" is asserted on the result, not on the storage.
opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/VLVIndex.java:271, ReplayedConfigChangeTest.java:680-683
The again call asserts SUCCESS, no admin action, no messages; delete the if (requiresRebuild) guard and it stays green. writes() (:955) is what pins it, as aChangeWhichLeavesTheBaseDNsAloneOpensNoTransaction already does at :542-547.
final int writesBefore = backend.storage.writes();
final ConfigChangeResult again = vlvIndex.applyConfigurationChange(vlvIndexCfg("+sn"));
assertThat(backend.storage.writes()).as("a change which changes nothing opens no transaction").isEqualTo(writesBefore);suggestion (non-blocking): The raised-limit case arms the untrust write by its ordinal.
opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/ReplayedConfigChangeTest.java:629
conflictAtCommitOnWrite(3, 1) names the third write, and the comment above it explains why it is the third. #994 restructures the same method; once a write is merged or dropped the arm lands on another write and the case stays green pinning something else. The cheapest guard turns that red:
final int writesBefore = backend.storage.writes();
backend.storage.conflictAtCommitOnWrite(3, 1);
// ...
assertThat(backend.storage.writes()).as("the armed write was the last of three").isEqualTo(writesBefore + 3);note (non-blocking): A failure of the untrust operation now leaves VLVIndex.applyConfigurationChange as an exception, and no case pins the road the PR claims.
opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/VLVIndex.java:271-296
Stated as deliberate at :274-279, recorded rather than argued: ConfigurationHandler.replaceEntry (:647-656) catches nothing, so on the give-up road and on any non-conflict failure the client gets the worker's uncaught-exception response instead of ERR_CONFIG_FILE_MODIFY_APPLY_FAILED. The blocking fix above closes the give-up half. What stays unpinned is the claim itself — "the conflict now reaches the retry loop": conflictAtFirstStorageAccess throws a raw RollbackException, nothing throws the StorageRuntimeException State.removeFlagsFromIndex produces from inside the operation. A harness point which does, plus attempts() == 2 and the flag gone, pins it:
writeOperation.run(txn);
throw new StorageRuntimeException(new RollbackException());note (non-blocking): index-entry-limit 0 means no limit, and < classifies it as the smallest limit.
opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/AttributeIndex.java:1044 (mirrors DefaultIndex.java:304; DefaultIndex.java:247 is where 0 means unlimited)
4000 → 0 answers "no rebuild" though every key over 4000 is undefined and stays so; 0 → 4000 answers "rebuild" though nothing needs one. Pre-existing — the PR copies the expression into planIndexUpdates — so a follow-up is fine; the lowered-limit case above is the case it needs.
final int oldLimit = updatedIndex.getIndexEntryLimit();
final int newLimit = newConfig.getIndexEntryLimit();
final boolean newLimitRequiresRebuild = oldLimit != 0 && (newLimit == 0 || oldLimit < newLimit);
…hange outside the write which is replayed AttributeIndex.applyConfigurationChange called two helpers from inside a WriteOperation, and both wrote into the caller's ConfigChangeResult and mutated in-memory state no rollback undoes. Storage.write() may replay a rolled back operation - PDBStorage and JDBCStorage both do - so the operator was told to rebuild an index once per attempt, and setIndexEntryLimit(), which answers "the limit went up" by comparing the new limit with a field it then assigns, left the replay comparing the raised limit with itself: it found nothing to rebuild and committed an index whose entry limit was raised while the TRUSTED flag the rollback had put back was still stored. DefaultIndex.afterOpen reads that flag back after a restart and the index is used as if it were complete. The decision is now taken before the write, the write only removes the flag, and the entry limit and the messages are applied once it has committed - the shape EntryContainer.applyConfigurationAdd already uses for the index it adds. VLVIndex.applyConfigurationChange had the same shape, and the only thing keeping its replay from committing a trusted vlvIndex was the administrative action the rolled back attempt had left in the shared result - the same accident that repeated the message. It is decided before the write and published after it as well, and a conflict raised by the flag removal now reaches the retry loop instead of being caught and reported.
…report a give-up on the vlvIndex flag removal The first round reported the rebuild a raised entry limit needs only once the write which untrusts the index had committed. A write the storage gives up on - the bound of its retry loop spent, or a failure it does not replay at all - dropped that instruction together with the limit it could not apply, while config.ldif already held the raised limit: ConfigurationHandler .replaceEntry writes it before the listeners run and nothing rolls it back. The next open of the index applied the limit to a tree whose keys were given up under the lower one, with TRUSTED still stored, and the operator had been told nothing but a stack trace: the OpenIdentityPlatform#991 end state, on a road where the base delivered the message by the same accident which repeated it. What the indexes which stay are asked for is now decided and reported before any of the three writes, since the instruction holds whichever way they go, and only the limit itself waits for the untrust write to commit. That write is made only when there is something to untrust, as VLVIndex already did: a lowered limit opens no transaction a bounded storage could give up on. VLVIndex reported a give-up by throwing, which discarded the result carrying the rebuild it had asked for - ConfigurationHandler catches nothing a listener throws. It now reports it the way AttributeIndex does, with the message asked for before the write as well. Pinned: both give-up roads; a lowered limit, which needs no rebuild and opens no transaction; the write ordinals the replay cases arm; and the conflict the flag removal itself raises, in the form the PDB transaction raises it - a StorageRuntimeException wrapping the RollbackException, which the inner catch the first round removed used to swallow, and which the bare form the harness raised never reached.
ed6607c to
b7094cd
Compare
|
Round 2 pushed as b7094cd, on top of a rebase to current master 129fc4e (range-diff of the first commit is issue (blocking) — the rebuild instruction was delivered only if the write committed. Verified as described: any failure of a write — the bound spent, or a failure the engine does not replay at all ( On "write 1 has the same shape": the answer there needs the transaction (
Pins: suggestion — the untrust write with nothing to do. Guarded, third write only: suggestion — no case lowers the limit. suggestion — the suggestion — the arm by ordinal. note — "the conflict now reaches the retry loop" was unpinned. Agreed, and pinned — with a different harness point than the one in the note, because that one does not tell the base from the head. A throw placed after note — Runs: |
2 participants
Fixes #991.
AttributeIndex
applyConfigurationChangecalledcreateIndexandupdateIndexfrom inside aWriteOperation, and both wrote into the caller'sConfigChangeResultand mutated in-memory state no rollback undoes.Storage.write()may replay a rolled back operation —PDBStorageandJDBCStorageboth do — so:setIndexEntryLimit()answers "the limit went up" by comparing the new limit with a field it then assigns, so the replay compared the raised limit with itself, found nothing to rebuild, and committed an index whose entry limit was raised while theTRUSTEDflag the rollback had put back was still stored.DefaultIndex.afterOpenreads that flag back after a restart and the index is used as if it were complete.Now what the indexes which stay are asked for is decided before any of the three writes —
updateIndexbecomesplanIndexUpdates, which readsgetIndexEntryLimit()rather than the return value of the setter — and reported there as well, because the instruction holds whichever way the writes go:ConfigurationHandler.replaceEntryhas already written the raised limit to config.ldif when the listener runs, and the next open of the index applies it to a tree whose keys were given up under the lower one. A write the storage gives up on therefore leaves the rebuild instruction in the result next to the failure, where the first round dropped it together with the limit. The untrust write does onlysetTrusted(txn, false), and only when there is something to untrust — a lowered limit opens no transaction, asVLVIndexalready did not; the entry limit is applied once that write has committed.createIndexanswers whether the index it opened has to be rebuilt instead of reporting it, and the report follows the write, asEntryContainer.applyConfigurationAdd:204-225already does for the index it adds and for the same reason: that answer needs the transaction.VLVIndex
VLVIndex.applyConfigurationChangehad the same shape and is included here rather than tracked on its own, since the analysis and the fix are one and the same. Every question it asked was asked of the configuration it holds, and both that configuration and the four fields around it were assigned inside the write, so a replay finds nothing changed.Worth stating precisely, because it is not what one would expect: the replay does still remove the flag today, and what keeps it doing so is that the rolled back attempt left
adminActionRequiredin the sharedConfigChangeResult— the same accident that repeats the message. The observable defect is therefore the duplicated message; the persisted flag rides on that duplication. Decided and reported before the write and published after it, neither depends on the other any more.A conflict raised by the flag removal itself was also caught inside the operation and turned into a message, so the storage was never told to replay it and committed an attempt which had done nothing. It now reaches the retry loop. What the storage gives up on is reported the way
AttributeIndexreports it — server error, the stack trace as a message — with the result built so far, rather than thrown:ConfigurationHandlercatches nothing a listener throws and would discard the rebuild the result asks for together with the exception.Tests
Seven tests in
ReplayedConfigChangeTest, which already carries the replaying storage from #907:anIndexAddedByAChangeIsReportedOnceWhenTheTransactionIsReplayedaRaisedEntryLimitUntrustsTheIndexWhenTheTransactionIsReplayedaRaisedEntryLimitIsReportedWhenTheWriteWhichUntrustsTheIndexGivesUpaLoweredEntryLimitNeedsNoRebuildAndOpensNoTransactionaChangedSortOrderUntrustsTheVlvIndexWhenTheTransactionIsReplayedaChangedSortOrderIsReportedWhenTheWriteWhichUntrustsTheVlvIndexGivesUpaConflictRaisedByTheRemovalOfTheVlvIndexFlagIsReplayedThe harness gains
conflictAtCommitOnWrite(nth, conflicts)andfailWithoutReplayOnWrite(nth), since an index change makes three writes and each test has to arm the one it is about;conflictAsTheTransactionReportsIt(conflicts), which raises the first-access conflict in the formPDBStorage's own transaction raises it — aStorageRuntimeExceptionwrapping theRollbackException, the only form an inner catch of the former ever met; and a vlvIndex to open where a test asks for one.Red at the base for the assertion each is about: two messages instead of one;
[TRUSTED, COMPACTED]still stored after the raised limit; the vlvIndex conflict swallowed (one attempt, server error, flag intact); the give-up roads answered with the limit an attempt which rolled back had assigned, or with an exception in place of a result. Green here:ReplayedConfigChangeTest19/19, andPDBTestCase,DefaultIndexTest,BulkCursorTest,OnDiskMergeImporterTest,VLVControlTestCase,ServerSideSortControlTestCase— 133 tests, no failures.Not in this change
index-entry-limit0 means no limit, and the comparisonplanIndexUpdatesinherits fromDefaultIndex.setIndexEntryLimitclassifies it as the smallest limit. Pre-existing; filed as #1059 rather than folded in, since the method is already shared by #994 and #1000 in flight.Note for the merge
#994 restructures the same method for #962 — it moves the publication of
config,indexingOptionsandindexIdToIndexesaround the same three writes — and #1000 restructures it again for #992, takingsetConfidentialout ofIndex, whichplanIndexUpdatescalls. The three conflict textually and whichever land later will need the rebase.